Skip to content

feat: Support clicks and custom event tracing#432

Open
SpennyNDaJets wants to merge 6 commits intomainfrom
spenny/support-product-analytics-tracing
Open

feat: Support clicks and custom event tracing#432
SpennyNDaJets wants to merge 6 commits intomainfrom
spenny/support-product-analytics-tracing

Conversation

@SpennyNDaJets
Copy link
Copy Markdown
Contributor

@SpennyNDaJets SpennyNDaJets commented Mar 25, 2026

Summary

Support tracing for the following product analytics events

  • Clicks
  • PageViews
  • LD.track

Make sure to add the LD context to each of these traces

Note:

  • Removes support for otel eventNames
  • Passes in full url for clicks to better support search

How did you test this change?

  1. Run the react router e2e app
  2. Make sure the client is running
  • Clicks are recorded with correct info
  • PageViews are recorded with correct info
  • Custom events are recorded with correct info

https://www.loom.com/share/e112659086f94a228279a97cc2b690b7

Are there any deployment considerations?

Breaking change: removal of event_names


Note

Medium Risk
Introduces new tracing behavior (click/page-view/track spans) and removes the otel.eventNames configuration, which is a breaking API change and could affect downstream instrumentation expectations.

Overview
Adds product analytics tracing across the observability SDK: click interactions, SPA page views, and LD.track now emit dedicated spans and include the active LaunchDarkly context keys as span attributes.

Replaces the old otel.eventNames knob with a new productAnalytics option (boolean or per-event toggles) and wires this through ObserveSDK/Highlight OTEL setup; LD.track additionally starts a launchdarkly.track span when enabled.

Updates the e2e React Router app with new LDClient demo routes (auto-start + manual-start) and points plugin backends/OTLP endpoints to staging to exercise the new spans.

Written by Cursor Bugbot for commit 3a31d36. This will update automatically on new commits. Configure here.

export const LD_INITIALIZE_EVENT = '$ld:telemetry:session:init'
export const LD_TRACK_EVENT = '$ld:telemetry:track'
export const LD_TRACK_SPAN_NAME = 'launchdarkly.track'
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How the name will be displayed in traces

})
}

this.observe?.recordLog('LD.track', 'info', trackAttrs)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still want a log?

@SpennyNDaJets SpennyNDaJets changed the title Feat: Support clicks and custom event tracing feat: Support clicks and custom event tracing Mar 25, 2026
@SpennyNDaJets SpennyNDaJets marked this pull request as ready for review March 26, 2026 15:00
@SpennyNDaJets SpennyNDaJets requested a review from a team as a code owner March 26, 2026 15:00
private _productAnalyticsEvents(): ProductAnalyticsEvents {
const pa = this._options?.productAnalytics
if (pa === false) {
return {}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabling product analytics still tracks clicks and page views

Medium Severity

When productAnalytics is false, _productAnalyticsEvents() returns {} (empty object). Downstream consumers check config.productAnalyticsEvents?.clicks !== false and config.productAnalyticsEvents?.pageViews !== false, which both evaluate to undefined !== falsetrue. This means click tracking and page view tracking remain enabled even when the user explicitly opts out by setting productAnalytics: false. The empty object needs to explicitly set clicks: false, pageViews: false, and trackEvents: false for the opt-out to propagate correctly.

Additional Locations (1)
Fix in Cursor Fix in Web

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct - they are on by default

}
}
return paEvents
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated _productAnalyticsEvents method across two files

Low Severity

_productAnalyticsEvents() is implemented identically in both ObserveSDK and the Highlight client class. This duplication means any bug fix (such as the productAnalytics: false issue) needs to be applied in two places, increasing the risk of divergent behavior over time. Extracting this into a shared utility function would reduce maintenance burden.

Additional Locations (1)
Fix in Cursor Fix in Web

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confused if it needs to be implemented in the highlight class but put it there to be safe

@@ -45,8 +45,13 @@ export const FEATURE_FLAG_VARIATION_INDEX_ATTR = `${FEATURE_FLAG_SCOPE}.result.v
export const FEATURE_FLAG_APP_ID_ATTR = `${LD_SCOPE}.application.id`
export const FEATURE_FLAG_APP_VERSION_ATTR = `${LD_SCOPE}.application.version`

export const PRODUCT_ANALYTICS_SCOPE = 'product_analytics'
export const PRODUCT_ANALYTICS_CONTEXT_ATTR = `${PRODUCT_ANALYTICS_SCOPE}.context_keys`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Do we want the context information under the product_analytics scope? At least in a spans attributes.

I imagine the context being a more generic attribute which we can add to spans across various use cases, so I prefer not locking it into PA even though it fits for this instance.

Maybe context.context_keys.<key>?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, open to changing this. I wasn't sure how important this was to be unique for processing the traces into the "fact tables", or how overloaded context may be (doesn't look like we are using it currently). May see if @mayberryzane has a preference?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree that context probably makes more sense than product_analytics since it's not PA specific

* User interaction instrumentation event names to record.
* Defaults to 'click', 'input', 'submit' window events.
*/
eventNames?: EventName[]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double checking, this is ok to remove?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Synced with @ccschmitz-launchdarkly and decided that we should remove support for this

const span = this.tracer.startSpan(LD_PAGE_VIEW_SPAN_NAME, {
attributes: {
[SemanticAttributes.ATTR_URL_FULL]: currentUrl,
'page_view.previous_url': previousUrl,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this useful?

@@ -168,4 +168,6 @@ export interface Observe {
environmentMetadata: LDPluginEnvironmentMetadata,
): void
getHooks?(metadata: LDPluginEnvironmentMetadata): Hook[]
setLDContextKeys(contextKeys: Attributes): void
getLDContextKeyAttributes(): Attributes | undefined
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (naming): why is it getLDContextKeyAttributes instead of getLDContextKeys? Or should we call the other one setLDContextKeyAttributes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can look into changing this - annoyingly we have to map through the entries to prefix with PRODUCT_ANALYTICS_CONTEXT_ATTR which this function does, so the data in != data out, but maybe not the best place to do so

* Specifies whether to record product analytics events.
* @default true
*/
productAnalytics?: boolean | ProductAnalyticsEvents
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dyozie-ld for any feedback on api

this._popstateHandler = () => plugin._checkLocation()
window.addEventListener('popstate', this._popstateHandler)

this._pollInterval = setInterval(() => plugin._checkLocation(), 300)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is polling necessary in addition to patching the history methods?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated to sync with the session replay logic but think the js-core logic had some holes in the patching

/** The location is watched via polling and popstate events because it is possible to miss
     * navigation at certain points with just popstate. It is also to miss events with polling
     * because they can happen within the polling interval.
     * Details on when popstate is called:
     * https://developer.mozilla.org/en-US/docs/Web/API/Window/popstate_event#when_popstate_is_sent
     */

https://github.com/launchdarkly/js-core/blob/main/packages/sdk/browser/src/goals/LocationWatcher.ts#L38-L43

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

There are 4 total unresolved issues (including 2 from previous reviews).

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

}
}
return paEvents
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated _productAnalyticsEvents method across two files

Medium Severity

The _productAnalyticsEvents() method is identically duplicated in both client/index.tsx and sdk/observe.ts. This duplication increases the risk that a bug fix (like the productAnalytics: false issue) gets applied in one location but missed in the other. Extracting this into a shared utility would avoid that.

Additional Locations (1)
Fix in Cursor Fix in Web

instrumentations: this.options?.otel?.instrumentations,
eventNames: this.options?.otel?.eventNames,
getIntegrations: () => [...this._integrations],
productAnalyticsEvents: this._productAnalyticsEvents(),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double PathListener causes duplicate Navigate custom events

Medium Severity

In client/index.tsx, setupBrowserTracing now receives productAnalyticsEvents (defaulting to all enabled), which causes a new LocationChangeInstrumentation to be registered. This instrumentation uses PathListener, which patches history.pushState/replaceState and dispatches locationchange events. But client/index.tsx already registers its own PathListener at line 1072. Two PathListener instances each wrap history methods, so every navigation dispatches locationchange twice. The LocationChangeInstrumentation deduplicates via _lastUrl, but the existing navigate callback has no dedup, producing duplicate "Navigate" custom events.

Additional Locations (1)
Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants